Skip to content

feat: improve Keycloak device auth page to include SSO options#2100

Merged
wilsonrivera merged 24 commits intomainfrom
wilson/eng-5025-keycloak-device-auth-page-should-show-sso-options
Sep 23, 2025
Merged

feat: improve Keycloak device auth page to include SSO options#2100
wilsonrivera merged 24 commits intomainfrom
wilson/eng-5025-keycloak-device-auth-page-should-show-sso-options

Conversation

@wilsonrivera
Copy link
Copy Markdown
Contributor

@wilsonrivera wilsonrivera commented Aug 2, 2025

Summary by CodeRabbit

  • New Features

    • SSO cookie-based authenticator, "Sign in with SSO" login option, and new localization string.
  • Enhancements

    • Keycloak and related tooling upgraded to 26.2.5; Java runtime bumped to Java 17.
    • New authentication flows, client scopes, required actions, HS512 key provider, and theme/login UI/styling tweaks.
  • Chores

    • Optional AUTH_SSO_COOKIE_DOMAIN env var added and wired into config; Keycloak admin client pinned to 26.2.5; dev setup script updated.

Checklist

Improve the device authentication page to include SSO options, this includes a custom Keycloak extension to read the Cosmo SSO cookie to provide the option to use organization SSO options

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Aug 2, 2025

Walkthrough

Adds optional AUTH_SSO_COOKIE_DOMAIN env var wired into config and AuthUtils; introduces a Keycloak SSO-cookie authenticator and factory with theme/template/localization and service registration; updates Keycloak realm, version, Dockerfile, setup script, Maven and package dependency versions.

Changes

Cohort / File(s) Change Summary
SSO cookie domain config
controlplane/.env.example, controlplane/src/core/env.schema.ts, controlplane/src/index.ts, controlplane/src/core/build-server.ts, controlplane/src/core/auth-utils.ts
Add optional AUTH_SSO_COOKIE_DOMAIN env var (empty -> undefined, regex-validated); propagate as ssoCookieDomain in BuildConfig and AuthUtilsOptions; AuthUtils.createSsoCookie uses ssoCookieDomain if provided, otherwise falls back to webDomain.
Keycloak realm and server versions
docker/keycloak/realm.json, keycloak/Dockerfile, .github/scripts/setup-keycloak.sh, controlplane/package.json
Major realm.json updates (new client scopes, auth flows, required actions, user profile provider, key provider, client scope/attribute edits, formatting); bump Keycloak version to 26.2.5; update Dockerfile base images and .github KC_VERSION; pin @keycloak/keycloak-admin-client to 26.2.5.
Custom Keycloak authenticator (Java)
keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticator.java, keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticatorFactory.java, keycloak/theme/src/main/resources/META-INF/services/org.keycloak.authentication.AuthenticatorFactory
Add SSOCookieAuthenticator and SSOCookieAuthenticatorFactory; service registration entry. Authenticator reads configured SSO cookie, resolves IDP, composes broker login URL, and exposes an SSO login link to the auth form context.
Theme, templates, localization, build props
keycloak/theme/src/main/resources/theme/cosmo/login/login.ftl, keycloak/theme/src/main/resources/theme/cosmo/login/messages/messages_en.properties, keycloak/theme/src/main/resources/theme/cosmo/login/theme.properties, keycloak/theme/pom.xml
Modify login template to conditionally render SSO login link and adjust social link classes; add signInWithSSO message; update theme CSS vendor paths and social section classes; add Maven properties/dependencies and bump theme version.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the primary change — adding SSO options to Keycloak's device authentication UI — and aligns with the code changes (Keycloak authenticator, theme/template updates, and env/config wiring); it is a single clear sentence and free of extraneous noise.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6def13a and 9a9567d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • controlplane/.env.example (1 hunks)
  • controlplane/package.json (1 hunks)
  • controlplane/src/core/env.schema.ts (1 hunks)
  • controlplane/src/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • controlplane/package.json
  • controlplane/src/index.ts
  • controlplane/.env.example
  • controlplane/src/core/env.schema.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_push_image
  • GitHub Check: build_push_image
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: image_scan
  • GitHub Check: integration_test (./events)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 2, 2025

Dependency Review

The following issues were found:
  • ❌ 1 vulnerable package(s)
See the Details below.

Vulnerabilities

keycloak/theme/pom.xml

NameVersionVulnerabilitySeverity
org.keycloak:keycloak-core25.0.2Keycloak mTLS Authentication Bypass via Reverse Proxy TLS Termination high
Only included vulnerabilities with severity high or higher.

OpenSSF Scorecard

PackageVersionScoreDetails
maven/org.keycloak:keycloak-core 25.0.2 🟢 9.3
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Maintained🟢 1030 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
CII-Best-Practices🟢 5badge detected: Passing
License🟢 10license file detected
Packaging⚠️ -1packaging workflow not detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: githubv4.Query: Resource not accessible by integration
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Security-Policy🟢 10security policy file detected
Signed-Releases🟢 85 out of the last 5 releases have a total of 5 signed artifacts.
Binary-Artifacts🟢 10no binaries found in the repo
SAST🟢 10SAST tool detected
Pinned-Dependencies🟢 7dependency not pinned by hash detected -- score normalized to 7
Fuzzing🟢 10project is fuzzed
Vulnerabilities🟢 82 existing vulnerabilities detected
maven/jakarta.ws.rs:jakarta.ws.rs-api 3.1.0 UnknownUnknown
maven/org.keycloak:keycloak-server-spi 25.0.2 🟢 9.3
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Maintained🟢 1030 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
CII-Best-Practices🟢 5badge detected: Passing
License🟢 10license file detected
Packaging⚠️ -1packaging workflow not detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: githubv4.Query: Resource not accessible by integration
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Security-Policy🟢 10security policy file detected
Signed-Releases🟢 85 out of the last 5 releases have a total of 5 signed artifacts.
Binary-Artifacts🟢 10no binaries found in the repo
SAST🟢 10SAST tool detected
Pinned-Dependencies🟢 7dependency not pinned by hash detected -- score normalized to 7
Fuzzing🟢 10project is fuzzed
Vulnerabilities🟢 82 existing vulnerabilities detected
maven/org.keycloak:keycloak-server-spi-private 25.0.2 🟢 9.3
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Maintained🟢 1030 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
CII-Best-Practices🟢 5badge detected: Passing
License🟢 10license file detected
Packaging⚠️ -1packaging workflow not detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: githubv4.Query: Resource not accessible by integration
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Security-Policy🟢 10security policy file detected
Signed-Releases🟢 85 out of the last 5 releases have a total of 5 signed artifacts.
Binary-Artifacts🟢 10no binaries found in the repo
SAST🟢 10SAST tool detected
Pinned-Dependencies🟢 7dependency not pinned by hash detected -- score normalized to 7
Fuzzing🟢 10project is fuzzed
Vulnerabilities🟢 82 existing vulnerabilities detected

Scanned Files

  • keycloak/theme/pom.xml

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (4)
controlplane/.env.example (1)

17-20: Provide an illustrative value for AUTH_SSO_COOKIE_DOMAIN

Leaving the variable blank encourages the empty-string issue mentioned earlier. Supplying a commented example helps users configure it correctly:

-AUTH_SSO_COOKIE_DOMAIN=
+# AUTH_SSO_COOKIE_DOMAIN=".example.com"   # optional – leading dot recommended
keycloak/theme/src/main/resources/theme/cosmo/login/messages/messages_en.properties (1)

6-7: Remember to update other locales

The new signInWithSSO key is only in messages_en.properties. Add it to the other language files to avoid missing-key fall-backs in non-English UIs.

keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticatorFactory.java (1)

71-75: Consider supporting additional authentication requirements.

Currently, only REQUIRED is supported. Most Keycloak authenticators also support ALTERNATIVE and DISABLED to provide more flexibility in authentication flow configuration.

 @Override
 public AuthenticationExecutionModel.Requirement[] getRequirementChoices() {
     return new AuthenticationExecutionModel.Requirement[]{
         AuthenticationExecutionModel.Requirement.REQUIRED,
+        AuthenticationExecutionModel.Requirement.ALTERNATIVE,
+        AuthenticationExecutionModel.Requirement.DISABLED
     };
 }
keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticator.java (1)

14-45: Add debug logging for troubleshooting.

Consider adding debug logging to help troubleshoot SSO cookie authentication issues in production environments.

Add logging at key points:

import org.jboss.logging.Logger;

public class SSOCookieAuthenticator implements Authenticator {
    private static final Logger logger = Logger.getLogger(SSOCookieAuthenticator.class);
    
    @Override
    public void authenticate(AuthenticationFlowContext authenticationFlowContext) {
        String ssoCookieName = getSSOCookieName(authenticationFlowContext);
        logger.debugf("Looking for SSO cookie: %s", ssoCookieName);
        
        // ... existing code ...
        
        if (idpModel != null && idpModel.isEnabled()) {
            logger.debugf("Found enabled IDP for cookie value: %s", ssoCookieValue);
            // ... existing code ...
        } else if (idpModel != null) {
            logger.debugf("IDP %s is disabled", ssoCookieValue);
        } else {
            logger.debugf("No IDP found for cookie value: %s", ssoCookieValue);
        }
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a947b1 and faf540b.

📒 Files selected for processing (14)
  • controlplane/.env.example (1 hunks)
  • controlplane/src/core/auth-utils.ts (2 hunks)
  • controlplane/src/core/build-server.ts (2 hunks)
  • controlplane/src/core/env.schema.ts (1 hunks)
  • controlplane/src/index.ts (2 hunks)
  • docker/keycloak/realm.json (59 hunks)
  • keycloak/Dockerfile (1 hunks)
  • keycloak/theme/pom.xml (1 hunks)
  • keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticator.java (1 hunks)
  • keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticatorFactory.java (1 hunks)
  • keycloak/theme/src/main/resources/META-INF/services/org.keycloak.authentication.AuthenticatorFactory (1 hunks)
  • keycloak/theme/src/main/resources/theme/cosmo/login/login.ftl (2 hunks)
  • keycloak/theme/src/main/resources/theme/cosmo/login/messages/messages_en.properties (1 hunks)
  • keycloak/theme/src/main/resources/theme/cosmo/login/theme.properties (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: oci (open container initiative) registry urls in the cosmo router project should not include http/ht...
Learnt from: endigma
PR: wundergraph/cosmo#2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.

Applied to files:

  • docker/keycloak/realm.json
🪛 GitHub Actions: Dependency Review
keycloak/theme/pom.xml

[error] 1-1: Dependency review detected vulnerable package: Keycloak mTLS Authentication Bypass via Reverse Proxy TLS Termination (high severity). See GHSA-93ww-43rr-79v3

🔇 Additional comments (15)
controlplane/src/index.ts (1)

100-105: Guard against empty string when forwarding ssoCookieDomain

If AUTH_SSO_COOKIE_DOMAIN is left blank in the environment file, env.schema currently passes '' down, so ssoCookieDomain: ''. Most cookie helpers interpret an empty domain as “set nothing” and drop the attribute, but some libraries treat the empty string literally and will emit Domain="", again breaking browser compatibility.

After tightening validation in env.schema.ts, this problem vanishes, but until then you can add a short-term guard:

-    ssoCookieDomain: AUTH_SSO_COOKIE_DOMAIN,
+    ssoCookieDomain: AUTH_SSO_COOKIE_DOMAIN || undefined,

(Not required if you adopt the schema fix.)

keycloak/Dockerfile (1)

1-1: Java 17 upgrade looks good; double-check Maven compatibility

Keycloak 25 requires Java 17, so switching the build stage image to java-node:17-jdk-18 is the right call.
No further issues spotted, but please ensure the Maven version pulled from apt still builds the theme without warnings under JDK 17.

keycloak/theme/src/main/resources/META-INF/services/org.keycloak.authentication.AuthenticatorFactory (1)

1-1: LGTM! Service provider configuration is correct.

This properly registers the SSOCookieAuthenticatorFactory for Keycloak's ServiceLoader mechanism, enabling automatic discovery of the custom authenticator.

controlplane/src/core/build-server.ts (2)

85-85: LGTM! Clean addition of SSO cookie domain configuration.

The optional ssoCookieDomain property is properly typed and placed in the appropriate auth configuration section.


239-239: LGTM! Proper propagation of SSO cookie domain to AuthUtils.

The new configuration option is correctly passed to the AuthUtils constructor, maintaining consistency with the existing configuration pattern.

controlplane/src/core/auth-utils.ts (2)

24-24: LGTM! Proper type extension for SSO cookie domain.

The optional ssoCookieDomain property is correctly typed and maintains backward compatibility.


116-116: LGTM! Excellent fallback implementation for SSO cookie domain.

The use of nullish coalescing (??) provides proper fallback to webDomain when ssoCookieDomain is undefined, maintaining backward compatibility while enabling flexible domain configuration.

keycloak/theme/pom.xml (1)

13-44: Maven configuration structure is well-organized.

The properties and dependencies sections are properly structured with appropriate scoping. The Java 11 compiler settings and UTF-8 encoding are correctly configured. However, address the Keycloak version security issue before merging.

keycloak/theme/src/main/resources/theme/cosmo/login/login.ftl (3)

7-7: LGTM! Proper client-specific social provider filtering.

The condition correctly excludes social providers when the client ID is "studio", providing appropriate UI customization.


10-10: LGTM! Consistent CSS class application.

The addition of kcFormSocialAccountLinkClass maintains consistency with the theme property system.


20-25: LGTM! Well-integrated SSO login option.

The conditional SSO login block is properly implemented with:

  • Appropriate conditional check for ssoLoginUrl
  • Consistent styling and icon usage
  • Proper localization with signInWithSSO message key
  • Maintains visual consistency with existing social providers
keycloak/theme/src/main/resources/theme/cosmo/login/theme.properties (1)

41-41: Consider preserving minimal styling for social account links.

Setting kcFormSocialAccountLinkClass to an empty string removes all styling from social account links. This might cause the SSO login link to appear unstyled or inherit unwanted styles from parent elements.

Consider keeping essential classes like pf-v5-u-text-decoration-none to maintain consistent link appearance across the login form.

docker/keycloak/realm.json (3)

1143-1156: Verify SMTP configuration for production use.

The SMTP server configuration has been changed to use a local Mailpit server (mailpit.hub-dev.orb.local). Ensure this is intentional and that proper SMTP configuration is used in production environments.

This appears to be a development configuration. Consider using environment-specific configuration for SMTP settings to avoid accidentally deploying with a local mail server.


1341-1354: New HMAC key provider with HS512 algorithm added.

A new HMAC key provider using HS512 algorithm has been added. This is good for supporting JWT signing with symmetric keys, but ensure this aligns with your security requirements and that the keys are properly managed.


2110-2110: Double-check custom authenticators & themes after upgrading Keycloak to 25.0.2

Updating from 22.0.3 → 25.0.2 includes several breaking changes that can impact your custom SPIs and theme overrides. Please audit the following before merging:

• File to review:

  • docker/keycloak/realm.json (line 2110):
    "keycloakVersion": "25.0.2",

• Custom authenticators:

  • Verify any session-state logic for updated error handling and session expiry propagation.
  • Ensure your SPI implementations account for claim changes (e.g., sub, auth_time, nonce now require explicit mappers).
  • Test flows that integrate passkeys or browser SSO to catch protocol adjustments.
    • Custom themes:
  • If you extended the legacy account-v1 or Angular admin console themes, migrate to the new React-based structures or use a compatibility extension.
  • Update theme.properties parent references to the current account/admin themes.
  • Compare all overridden FTL/CSS/JS against the Keycloak 25 defaults for removed resources or renamed template functions.
    • Configuration & protocol mappers:
  • Review hostname v2 settings if your flows reference frontend/backchannel URLs.
  • Audit custom protocol/token mappers against the new default client scopes and claim inclusion strategy.

Run a full smoke test of your authentication flows and theming in a staging environment, referring to the Keycloak Upgrade Guide and Release Notes for v23→25 to catch any additional issues.

Comment thread controlplane/src/core/env.schema.ts
Comment thread keycloak/theme/pom.xml Outdated
@wilsonrivera wilsonrivera requested review from a team as code owners August 4, 2025 16:48
@wilsonrivera wilsonrivera requested a review from Noroth August 4, 2025 16:48
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 4, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-ead0eb5c46c700cd0961df9afe540ab7b2cdbc92

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
docker/keycloak/realm.json (1)

1735-1758: Tidy up the flow alias naming

"alias": "browser with sso Browser - Conditional OTP" mixes two concepts and repeats the word Browser.
Consider a clearer alias such as "browser-with-sso-conditional-otp" (and update the single reference at Line 1788) to improve maintainability and avoid confusion when debugging flows.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0938aa and 4f2b6ea.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • .github/scripts/setup-keycloak.sh (1 hunks)
  • controlplane/package.json (1 hunks)
  • docker/keycloak/realm.json (62 hunks)
  • keycloak/Dockerfile (2 hunks)
  • keycloak/theme/pom.xml (1 hunks)
  • keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticator.java (1 hunks)
  • keycloak/theme/src/main/resources/theme/cosmo/login/theme.properties (2 hunks)
✅ Files skipped from review due to trivial changes (4)
  • controlplane/package.json
  • keycloak/Dockerfile
  • .github/scripts/setup-keycloak.sh
  • keycloak/theme/src/main/resources/theme/cosmo/login/theme.properties
🚧 Files skipped from review as they are similar to previous changes (2)
  • keycloak/theme/pom.xml
  • keycloak/theme/src/main/java/com/wundergraph/authentication/SSOCookieAuthenticator.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: oci (open container initiative) registry urls in the cosmo router project should not include http/ht...
Learnt from: endigma
PR: wundergraph/cosmo#2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.

Applied to files:

  • docker/keycloak/realm.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_test
  • GitHub Check: integration_test (./events)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)

Comment thread docker/keycloak/realm.json
Comment thread docker/keycloak/realm.json
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
docker/keycloak/realm.json (1)

642-651: Duplicate sub claim mapper still present

Keycloak injects the mandatory sub claim automatically; leaving this custom oidc-sub-mapper in place will create two sub entries in every token and break strict JWT validators.

The exact same issue was flagged earlier and hasn’t been addressed.
Please delete the entire mapper block.

🧹 Nitpick comments (1)
docker/keycloak/realm.json (1)

1788-1811: Inconsistent flow alias naming

"browser with sso Browser - Conditional OTP" mixes two concepts in one alias and differs in style from the existing Browser - Conditional OTP flows.
Keeping flow aliases short and consistent (e.g., browser-with-sso-otp) eases maintenance and scripting.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2b6ea and e105b55.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • .github/scripts/setup-keycloak.sh (1 hunks)
  • controlplane/package.json (1 hunks)
  • docker/keycloak/realm.json (62 hunks)
  • keycloak/Dockerfile (2 hunks)
  • keycloak/theme/pom.xml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • controlplane/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • keycloak/Dockerfile
  • keycloak/theme/pom.xml
  • .github/scripts/setup-keycloak.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: wilsonrivera
PR: wundergraph/cosmo#2100
File: docker/keycloak/realm.json:1768-1774
Timestamp: 2025-08-04T17:50:30.553Z
Learning: The sso-cookie-authenticator in Keycloak authentication flows must use "REQUIRED" as the requirement setting, not "ALTERNATIVE". This is due to implementation constraints of the custom SSOCookieAuthenticator class.
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: the sso-cookie-authenticator in keycloak authentication flows must use "required" as the requirement...
Learnt from: wilsonrivera
PR: wundergraph/cosmo#2100
File: docker/keycloak/realm.json:1768-1774
Timestamp: 2025-08-04T17:50:30.553Z
Learning: The sso-cookie-authenticator in Keycloak authentication flows must use "REQUIRED" as the requirement setting, not "ALTERNATIVE". This is due to implementation constraints of the custom SSOCookieAuthenticator class.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: oci (open container initiative) registry urls in the cosmo router project should not include http/ht...
Learnt from: endigma
PR: wundergraph/cosmo#2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.

Applied to files:

  • docker/keycloak/realm.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
  • GitHub Check: build_test
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: Analyze (go)
  • GitHub Check: integration_test (./events)
  • GitHub Check: image_scan
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
🔇 Additional comments (3)
docker/keycloak/realm.json (3)

1821-1826: sso-cookie-authenticator correctly set to REQUIRED – looks good

Matches the implementation constraint captured in project learnings; no action needed.


354-356: Verify that only cosmo-cli uses the new browser alt binding

Other public clients (e.g., studio, account) still rely on the default browser flow.
Double-check that this split is intentional and that UX remains uniform across entry points.


1476-1487: Multiple HMAC key providers share the same priority

Both hmac-generated (HS256) and the new hmac-generated-hs512 (HS512) use priority 100.
Keycloak picks the first matching provider, which may lead to unpredictable behaviour when HS* algorithms are requested.
Consider lowering the priority of the legacy HS256 provider—or removing it entirely—if HS512 is the intended algorithm.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
docker/keycloak/realm.json (1)

642-651: Duplicate sub protocol-mapper still present – remove to avoid double sub claims

Keycloak already injects the mandatory sub claim. Keeping the custom "sub" mapper here results in duplicate claims and breaks JWT validation in some libraries.

Same comment was raised in a previous review and still applies.
Delete the whole object at lines 642-651.

🧹 Nitpick comments (2)
docker/keycloak/realm.json (2)

1735-1759: Alias naming is confusing

"alias": "browser alt Browser - Conditional OTP" is hard to scan and easy to mistype.
Consider renaming to something concise like "browser alt - Conditional OTP".


1199-1207: basic scope added to default scopes but excluded from token.scope

"include.in.token.scope": "false" means the basic scope name itself will not be listed in the scope parameter, yet it is in defaultDefaultClientScopes.
That is valid but a little counter-intuitive—double-check this is intentional.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e105b55 and 1ef9b71.

📒 Files selected for processing (1)
  • docker/keycloak/realm.json (62 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: wilsonrivera
PR: wundergraph/cosmo#2100
File: docker/keycloak/realm.json:1768-1774
Timestamp: 2025-08-04T17:50:30.553Z
Learning: The sso-cookie-authenticator in Keycloak authentication flows must use "REQUIRED" as the requirement setting, not "ALTERNATIVE". This is due to implementation constraints of the custom SSOCookieAuthenticator class.
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: the sso-cookie-authenticator in keycloak authentication flows must use "required" as the requirement...
Learnt from: wilsonrivera
PR: wundergraph/cosmo#2100
File: docker/keycloak/realm.json:1768-1774
Timestamp: 2025-08-04T17:50:30.553Z
Learning: The sso-cookie-authenticator in Keycloak authentication flows must use "REQUIRED" as the requirement setting, not "ALTERNATIVE". This is due to implementation constraints of the custom SSOCookieAuthenticator class.

Applied to files:

  • docker/keycloak/realm.json
📚 Learning: oci (open container initiative) registry urls in the cosmo router project should not include http/ht...
Learnt from: endigma
PR: wundergraph/cosmo#2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.

Applied to files:

  • docker/keycloak/realm.json
🔇 Additional comments (3)
docker/keycloak/realm.json (3)

1693-1733: New top-level flow browser alt looks good, but confirm binding coverage

Only cosmo-cli is bound to this flow via authenticationFlowBindingOverrides.
Please verify that all device-code or browser clients expected to leverage the SSO-cookie authenticator are now bound to "browser alt"; otherwise they will silently fall back to the old flow.


1768-1775: sso-cookie-authenticator correctly set to REQUIRED

Requirement is REQUIRED, matching the implementation constraints recorded in project learnings. No action needed.


1423-1434: Unused HS512 key provider?

A new hmac-generated provider with algorithm HS512 is added, but defaultSignatureAlgorithm remains RS256 and no client appears to request HS512.

Please confirm this key is actually required; if not, drop it to keep the key set minimal.

…ould-show-sso-options

# Conflicts:
#	pnpm-lock.yaml
…auth-page-should-show-sso-options' into wilson/eng-5025-keycloak-device-auth-page-should-show-sso-options
@wilsonrivera wilsonrivera requested a review from SkArchon August 15, 2025 17:30
Copy link
Copy Markdown
Contributor

@SkArchon SkArchon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java code LGTM

Copy link
Copy Markdown
Contributor

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wilsonrivera wilsonrivera merged commit c0b4031 into main Sep 23, 2025
48 checks passed
@wilsonrivera wilsonrivera deleted the wilson/eng-5025-keycloak-device-auth-page-should-show-sso-options branch September 23, 2025 15:48
@Noroth Noroth mentioned this pull request Sep 30, 2025
5 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Mar 10, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants